Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some minor docker improvements #462

Merged
merged 4 commits into from
Jan 30, 2024

Conversation

g2flyer
Copy link
Contributor

@g2flyer g2flyer commented Jan 16, 2024

A few minor improvements in docker code to

  • docker & docker-compose consistent
    • consistent tagging
    • build only via docker to prevent double builds
    • consistent support of ADD_APT_PKGS
  • some additional docker related make targets, e.g., additional stop targets, test without complete rebuild and _dev targets to setup development containers
  • some minor dockerfile changes to be more consistent

BTW: in readme i've noticed we suggest in our examples to put user-name into container names. This seems a wise idea for multi-user settings. Should we also adopt it in the makefiles? Easy to do but haven't changed it yet as i'm not sure if some other places count on the container names being what they are ...
Along the lines, the might include in name in container (and, more importantly, image) also SGX-mode (where also hw-mode in testing seems to have gone lost but see also PR#463, interpreter, ledger, ...?

@g2flyer g2flyer requested a review from cmickeyb January 16, 2024 20:05
Copy link
Contributor

@cmickeyb cmickeyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall comment: please be consistent with "{}" or "()" for variable expansion in the Makefile. I don't have a strong feeling either way, but I would strongly prefer to go with one form or the other.

docker/pdo_client.dockerfile Show resolved Hide resolved
docker/pdo_client.dockerfile Outdated Show resolved Hide resolved
docker/pdo_client.dockerfile Show resolved Hide resolved
@cmickeyb cmickeyb self-assigned this Jan 17, 2024
@cmickeyb cmickeyb force-pushed the msteiner.docker-improve branch from 0b9ecad to 155883c Compare January 19, 2024 19:11
@g2flyer g2flyer requested review from bvavala and cmickeyb January 19, 2024 23:54
@g2flyer
Copy link
Contributor Author

g2flyer commented Jan 23, 2024

@cmickeyb Mic, i reworked the PR after our discussion, the PR description is now updated to the current changes (which essentially should address your comments as mentioned above)

Copy link
Member

@bvavala bvavala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, just one comment.

docker/pdo_client.dockerfile Outdated Show resolved Hide resolved
docker/pdo_services.dockerfile Outdated Show resolved Hide resolved
docker/services_base.yaml Show resolved Hide resolved
@g2flyer g2flyer force-pushed the msteiner.docker-improve branch from f61f416 to 48829e2 Compare January 23, 2024 03:23
bin/lib/common.sh Outdated Show resolved Hide resolved
docker/Makefile Show resolved Hide resolved
docker/Makefile Show resolved Hide resolved
docker/Makefile Show resolved Hide resolved
docker/Makefile Outdated Show resolved Hide resolved
docker/base.yaml Show resolved Hide resolved
docker/pdo_ccf.dockerfile Show resolved Hide resolved
docker/pdo_ccf.dockerfile Show resolved Hide resolved
docker/pdo_client.dockerfile Outdated Show resolved Hide resolved
docker/pdo_services.dockerfile Outdated Show resolved Hide resolved
bin/lib/common.sh Outdated Show resolved Hide resolved
docker/pdo_client.bashrc Outdated Show resolved Hide resolved
@g2flyer g2flyer force-pushed the msteiner.docker-improve branch from 3b65255 to af054f9 Compare January 24, 2024 20:25
@g2flyer g2flyer requested a review from cmickeyb January 24, 2024 20:59
@g2flyer g2flyer force-pushed the msteiner.docker-improve branch 2 times, most recently from 1996be2 to 1f95831 Compare January 29, 2024 18:55
@g2flyer
Copy link
Contributor Author

g2flyer commented Jan 29, 2024

@cmickeyb the latest version should address your comments and merge in the changes from your branch, with slight modifications for make.dev ...

@g2flyer g2flyer force-pushed the msteiner.docker-improve branch from 1f95831 to 6b8b36a Compare January 29, 2024 19:43
Copy link
Member

@bvavala bvavala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

g2flyer and others added 3 commits January 29, 2024 12:45
- build only via docker (& make) and not also with docker-compose
- consistent tagging
- consistent support of ADD_APT_PKGS
- some additional stop/clean-up targets

Signed-off-by: g2flyer <[email protected]>
@g2flyer g2flyer force-pushed the msteiner.docker-improve branch from 6b8b36a to 0fbd3ce Compare January 29, 2024 20:45
@g2flyer g2flyer force-pushed the msteiner.docker-improve branch from 5268461 to 7ee7223 Compare January 29, 2024 22:59
@cmickeyb cmickeyb merged commit 123b318 into hyperledger-labs:main Jan 30, 2024
4 checks passed
@g2flyer g2flyer deleted the msteiner.docker-improve branch January 30, 2024 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants